[DDC-3382] Allow orphan removal to be cancelled #1419

c960657 · 2015-06-11T20:36:46Z

I have a one-to-many relation with orphanRemoval=true.
If I remove an entity from the related collection and add it back, the entity is removed from the database.

$employee = $company->getEmployees()->first();
$company->getEmployees()->removeElement($employee);
$company->getEmployees()->add($employee);

$em->persist($company);
$em->flush();
// Now $employee is deleted from the database.

The expected behaviour is to leave the entity in the database, because it was present in the PersistentCollection when $em->persist() was called.

This has previously been suggested in DDC-3382, but it was rejected. This PR shows how small a change it is, so I hope the suggestion will be reconsidered in the light of this.

doctrinebot · 2015-06-11T20:37:01Z

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3765

We use Jira to track the state of pull requests and the versions they got
included in.

DHager · 2015-06-12T17:37:25Z

Hmmm, it would be nice to let entities to be safely "transferred" to another parent of the very same entity-type:

$employee = $oldCompany->getEmployees()->first();
$oldCompany->getEmployees()->removeElement($employee);
$newCompany->getEmployees()->add($employee);

However, isn't this change a little broader than that? It looks like the instant the child gets put in any PersistentCollection its removal is cancelled.

c960657 · 2015-06-12T21:22:37Z

@DHager Isn't that the expected behaviour? If the entity is added to another PersistentCollection, it is no longer an orphan and should thus not be deleted?

beberlei · 2015-06-16T19:19:26Z

Very good catch.

[DDC-3382] Allow orphan removal to be cancelled

beberlei · 2015-06-16T19:32:20Z

Waiting for @Ocramius ack to merge this to 2.5 branch

DHager · 2015-06-16T20:11:40Z

@c960657 : I guess I had the idea that orphanRemoval was also an implicit promise that the child was part of a subtree that could only be kept around by a parent of the appropriate type.

In that case, don't we also need to cancel the removal for orphans that are re-adopted directly by an entity, without involving a Collection? For example:

$company->getEmployees()->removeElement($person);
$company->setOwner($person);

I think there should be a test-case for that, and my guess is that it won't (yet) work.

Ocramius · 2015-06-16T22:23:51Z

Not OK for 2.5.1, as this behavior existed for a lot of versions before that, and even if buggy, a lot of users coded their entity logic according to that.

Charlie-Lucas · 2015-07-09T16:07:18Z

According to @Ocramius this pull request give me error when i try to update a collection with a new item. spl_object_hash() expects parameter 1 to be object, null given

c960657 · 2015-07-09T16:31:57Z

@Charlie-Lucas Could you provide steps to reproduce this problem?

Charlie-Lucas · 2015-07-09T16:58:21Z

I have a basic relation OneToMany with OrphanRemoval=true.
My form can send post values like

myForm[rows][0][name]:Row1
myForm[rows][1][name]:Row2

When i persist the first time my form with the entityManager, i have no errors.
But when i try edit this collection with a new item:

myForm[rows][0][name]:Row1
myForm[rows][1][name]:Row2
myForm[rows][2][name]:Row3

I have no controller logic to handle my collection, just a simple persist of my entity.
The cancelOrphanRemoval method is call with null as argument.

nea · 2015-07-29T09:55:15Z

Hi everyone

We face an issue with the extensive use of collections and the newly added cancelOrphanRemoval function.

unset($this->orphanRemovals[spl_object_hash($entity)]);

If $entity is 'null' for any reason in add/set this will fail with an exception. A check before to validate $entity is not null is at least helpful before unset.

Thanks

c960657 · 2015-07-29T20:08:16Z

Could you provide a stack trace for this error? It seems that something is setting or adding a null value to the persistent collection. That sounds like a bug to me.

DHager · 2015-07-29T22:02:15Z

@c960657 Can you create a test-case for when an entity is removed from a Collection but re-linked into the object-graph without involving another Collection? Ex:

$company->getEmployees()->removeElement($person);
$company->setOwner($person);

As I said before, I'm reasonably certain this will highlight a scenario that hasn't been covered yet, where $person will still be considered an orphan.

whitewhidow · 2015-09-14T10:54:15Z

Hi everyone
We face an issue with the extensive use of collections and the newly added cancelOrphanRemoval function.
unset($this->orphanRemovals[spl_object_hash($entity)]);
If $entity is 'null' for any reason in add/set this will fail with an exception. A check before to validate > $entity is not null is at least helpful before unset.
Thanks

I have this exact same issue me thinks

stack trace below:

[1] Symfony\Component\Debug\Exception\ContextErrorException: Warning: spl_object_hash() expects parameter 1 to be object, null given
    at n/a
        in /home/sam/git/PosBranch/vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php line 2446

    at Symfony\Component\Debug\ErrorHandler->handleError('2', 'spl_object_hash() expects parameter 1 to be object, null given', '/home/sam/git/PosBranch/vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php', '2446', array('entity' => null))
        in  line 

    at spl_object_hash(null)
        in /home/sam/git/PosBranch/vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php line 2446

    at Doctrine\ORM\UnitOfWork->cancelOrphanRemoval(null)
        in /home/sam/git/PosBranch/vendor/doctrine/orm/lib/Doctrine/ORM/PersistentCollection.php line 475

    at Doctrine\ORM\PersistentCollection->set('2', null)
        in /home/sam/git/PosBranch/vendor/doctrine/orm/lib/Doctrine/ORM/PersistentCollection.php line 522

    at Doctrine\ORM\PersistentCollection->offsetSet('2', null)
        in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/PropertyAccess/PropertyAccessor.php line 223

    at Symfony\Component\PropertyAccess\PropertyAccessor->readPropertiesUntil(object(PersistentCollection), object(PropertyPath), '1', true)
        in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/PropertyAccess/PropertyAccessor.php line 60

    at Symfony\Component\PropertyAccess\PropertyAccessor->getValue(object(PersistentCollection), object(PropertyPath))
        in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php line 58

    at Symfony\Component\Form\Extension\Core\DataMapper\PropertyPathMapper->mapDataToForms(object(PersistentCollection), object(RecursiveIteratorIterator))
        in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/Form/Form.php line 936

    at Symfony\Component\Form\Form->add('2', 'sonata_type_admin', array('sonata_field_description' => object(FieldDescription), 'data_class' => 'Sam\PosBundle\Entity\ArticlePrice', 'delete' => false, 'property_path' => '[2]', 'delete_options' => array('type' => 'hidden', 'type_options' => array('mapped' => false, 'required' => false))))
        in /home/sam/git/PosBranch/vendor/sonata-project/core-bundle/Form/EventListener/ResizeFormListener.php line 163

    at Sonata\CoreBundle\Form\EventListener\ResizeFormListener->preSubmit(object(FormEvent))
        in /home/sam/git/PosBranch/vendor/sonata-project/core-bundle/Form/EventListener/ResizeFormListener.php line 118

    at Sonata\CoreBundle\Form\EventListener\ResizeFormListener->preBind(object(FormEvent), 'form.pre_bind', object(EventDispatcher))
        in  line 

    at call_user_func(array(object(ResizeFormListener), 'preBind'), object(FormEvent), 'form.pre_bind', object(EventDispatcher))
        in /home/sam/git/PosBranch/app/cache/dev/classes.php line 1826

    at Symfony\Component\EventDispatcher\EventDispatcher->doDispatch(array(array(object(BindRequestListener), 'preBind'), array(object(TrimListener), 'preSubmit'), array(object(CsrfValidationListener), 'preSubmit'), array(object(ResizeFormListener), 'preBind')), 'form.pre_bind', object(FormEvent))
        in /home/sam/git/PosBranch/app/cache/dev/classes.php line 1759

    at Symfony\Component\EventDispatcher\EventDispatcher->dispatch('form.pre_bind', object(FormEvent))
        in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/EventDispatcher/ImmutableEventDispatcher.php line 43

    at Symfony\Component\EventDispatcher\ImmutableEventDispatcher->dispatch('form.pre_bind', object(FormEvent))
        in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/Form/Form.php line 556

    at Symfony\Component\Form\Form->submit(array(array('amount' => '11', 'articleSize' => '175'), array('amount' => '12', 'articleSize' => '176'), array('amount' => '13', 'articleSize' => '176')), true)
        in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/Form/Form.php line 577

    at Symfony\Component\Form\Form->submit(array('name' => 'ONLY PEPPERONI', 'frontName' => 'only pepperoni', 'articleCategory' => '85', 'weight' => '1', 'isVisibleInFront' => '1', 'isPurchaseable' => '1', 'articlePrices' => array(array('amount' => '11', 'articleSize' => '175'), array('amount' => '12', 'articleSize' => '176'), array('amount' => '13', 'articleSize' => '176')), '_token' => 'XAwEGLnw0a2hWWdOVracSwckqM9mDCcvkqDllrLcBHg'), true)
        in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/Form/Extension/HttpFoundation/HttpFoundationRequestHandler.php line 116

    at Symfony\Component\Form\Extension\HttpFoundation\HttpFoundationRequestHandler->handleRequest(object(Form), object(Request))
        in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/Form/Form.php line 499

    at Symfony\Component\Form\Form->handleRequest(object(Request))
        in /home/sam/git/PosBranch/vendor/sonata-project/admin-bundle/Controller/CRUDController.php line 407

    at Sonata\AdminBundle\Controller\CRUDController->editAction('154', object(Request))
        in  line 

    at call_user_func_array(array(object(CRUDController), 'editAction'), array('154', object(Request)))
        in /home/sam/git/PosBranch/app/bootstrap.php.cache line 3109

    at Symfony\Component\HttpKernel\HttpKernel->handleRaw(object(Request), '1')
        in /home/sam/git/PosBranch/app/bootstrap.php.cache line 3071

    at Symfony\Component\HttpKernel\HttpKernel->handle(object(Request), '1', true)
        in /home/sam/git/PosBranch/app/bootstrap.php.cache line 3222

    at Symfony\Component\HttpKernel\DependencyInjection\ContainerAwareHttpKernel->handle(object(Request), '1', true)
        in /home/sam/git/PosBranch/app/bootstrap.php.cache line 2444

    at Symfony\Component\HttpKernel\Kernel->handle(object(Request))
        in /home/sam/git/PosBranch/web/app_dev.php line 21

Any help would be greatly appreciated, as this is a mayor blocker for me atm, as i cant get past it (other then circumventing the orphanremoval code)

Let me know what steps i can take to help solve this issue.

c960657 · 2015-09-14T11:21:04Z

The problem occurs because a non-object is added to the PersistentCollection:

at Doctrine\ORM\PersistentCollection->set('2', null)
    in /home/sam/git/PosBranch/vendor/doctrine/orm/lib/Doctrine/ORM/PersistentCollection.php line 522

at Doctrine\ORM\PersistentCollection->offsetSet('2', null)
    in /home/sam/git/PosBranch/vendor/symfony/symfony/src/Symfony/Component/PropertyAccess/PropertyAccessor.php line 223

I don't know if this is supposed to work, but I don't think so. The documentation states that a "PersistentCollection represents a collection of elements that have persistent state". I believe this excludes non-elements.

The below code would throw a similar error even before this change:

        $doors = $house->getDoors();
        $doors[1] = null;
        $doors->clear();

So I don't think the bug is in Doctrine 2 but probably in the Symfony form binding code.

That being said, it seems reasonable to modify to add() and set() to check for non-object values. Because of BC we have to just silently ignore these (this might change in a future major release). I will prepare a PR for that.

whitewhidow · 2015-09-14T12:04:29Z

yes i know its adding a null to the collection, i just cant seem to figure out where, and why, things are going wrong, causing it to end up with this null.

Only thing i can do atm is overwrite the PropertyAccessor->getValue function with

        if (!$propertyPath instanceof PropertyPathInterface) {
            $propertyPath = new PropertyPath($propertyPath);
        }

        try {
            $propertyValues = &$this->readPropertiesUntil(
                $objectOrArray,
                $propertyPath,
                $propertyPath->getLength(),
                $this->ignoreInvalidIndices
            );
        }catch(\Exception $e) {
            return;
        }
        return $propertyValues[count($propertyValues) - 1][self::VALUE];

To make things atleast function

whitewhidow · 2015-09-14T12:22:48Z

temp workaround:

in app/config/config.yml:

property_accessor.class: BBIT\BlogBundle\Component\PropertyAccess\MyPropertyAccessor

in BBIT\BlogBundle\Component\PropertyAccess\MyPropertyAccessor

<?php
namespace BBIT\BlogBundle\Component\PropertyAccess;

use \Symfony\Component\PropertyAccess\PropertyAccessor as BaseAccessor;

class MyPropertyAccessor extends BaseAccessor
{
    public function getValue($objectOrArray, $propertyPath)
    {
        try {
            return parent::getValue($objectOrArray, $propertyPath);
        }catch(\Exception $e) {
            return;
        }
    }
}

grachevko · 2016-02-05T13:02:26Z

When this mergin will be released? It is very necessary.

vlastv · 2016-04-04T14:15:43Z

Ping...

#4785

Charlie-Lucas · 2016-04-04T14:56:13Z

@vlastv you must use 2.5 version of doctrine not dev-master

grachevko · 2016-05-24T15:49:32Z

@Charlie-Lucas 2.5 version does not contain this pr.

Charlie-Lucas · 2016-05-24T16:22:36Z

@grachevko sorry i was wrong this PR is only merged on master

c960657 force-pushed the cancel-orphan-removal branch 8 times, most recently from c2c5d3d to ecc5211 Compare June 12, 2015 10:51

[DDC-3382] Allow orphan removal to be cancelled

96dbece

c960657 force-pushed the cancel-orphan-removal branch from ecc5211 to 96dbece Compare June 12, 2015 14:44

beberlei added a commit that referenced this pull request Jun 16, 2015

Merge pull request #1419 from c960657/cancel-orphan-removal

1e7e8f2

[DDC-3382] Allow orphan removal to be cancelled

beberlei merged commit 1e7e8f2 into doctrine:master Jun 16, 2015

c960657 mentioned this pull request Sep 14, 2015

Ignore non-objects #1508

Closed

This was referenced Dec 6, 2015

DDC-3382: [GH-1419] With orphanRemoval, cannot delete and re-add entity #4184

Closed

DDC-3765: [GH-1419] [DDC-3382] Allow orphan removal to be cancelled #4606

Closed

DDC-3902: [GH-1508] Ignore non-objects #4759

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DDC-3382] Allow orphan removal to be cancelled #1419

[DDC-3382] Allow orphan removal to be cancelled #1419

c960657 commented Jun 11, 2015

doctrinebot commented Jun 11, 2015

DHager commented Jun 12, 2015

c960657 commented Jun 12, 2015

beberlei commented Jun 16, 2015

beberlei commented Jun 16, 2015

DHager commented Jun 16, 2015

Ocramius commented Jun 16, 2015

Charlie-Lucas commented Jul 9, 2015

c960657 commented Jul 9, 2015

Charlie-Lucas commented Jul 9, 2015

nea commented Jul 29, 2015

c960657 commented Jul 29, 2015

DHager commented Jul 29, 2015

whitewhidow commented Sep 14, 2015

c960657 commented Sep 14, 2015

whitewhidow commented Sep 14, 2015

whitewhidow commented Sep 14, 2015

grachevko commented Feb 5, 2016

vlastv commented Apr 4, 2016

Charlie-Lucas commented Apr 4, 2016

grachevko commented May 24, 2016

Charlie-Lucas commented May 24, 2016

[DDC-3382] Allow orphan removal to be cancelled #1419

[DDC-3382] Allow orphan removal to be cancelled #1419

Conversation

c960657 commented Jun 11, 2015

doctrinebot commented Jun 11, 2015

DHager commented Jun 12, 2015

c960657 commented Jun 12, 2015

beberlei commented Jun 16, 2015

beberlei commented Jun 16, 2015

DHager commented Jun 16, 2015

Ocramius commented Jun 16, 2015

Charlie-Lucas commented Jul 9, 2015

c960657 commented Jul 9, 2015

Charlie-Lucas commented Jul 9, 2015

nea commented Jul 29, 2015

c960657 commented Jul 29, 2015

DHager commented Jul 29, 2015

whitewhidow commented Sep 14, 2015

c960657 commented Sep 14, 2015

whitewhidow commented Sep 14, 2015

whitewhidow commented Sep 14, 2015

grachevko commented Feb 5, 2016

vlastv commented Apr 4, 2016

Charlie-Lucas commented Apr 4, 2016

grachevko commented May 24, 2016

Charlie-Lucas commented May 24, 2016